feat(testing): in-qemu tests #59

mystor · 2020-01-03T05:20:04Z

A bit sketchy, but that's the nature of custom test harnesses like this.
Built on top of #58, and doesn't work on gh-actions yet.

hawkw · 2020-01-03T19:29:00Z

.github/workflows/rust.yml

+      with:
+        command: dev-env
+    - name: install qemu
+      run: sudo apt-get install qemu


i wonder if this would still fail if we changed it to

Suggested change

run: sudo apt-get install qemu

run: sudo apt-get update && sudo apt-get install qemu

hawkw · 2020-01-04T02:24:00Z

whoa it appears to "work" now: https://github.com/hawkw/mycelium/runs/373304967#step:6:236

mystor · 2020-01-04T02:24:06Z

Yup, seems like that did it, @hawkw.
Still some clippy failures due to cfgs not being fully consistent (as the crate is now only built outside of the custom target for clippy). We could probably fix either by removing the cfg(target_os = "none") checks, or by adding those cfgs in a few more places.

hawkw · 2020-01-04T02:24:30Z

should we maybe have a separate build test for in-qemu tests vs crate unit tests?

hawkw · 2020-01-04T21:40:41Z

it looks like there's some code that needs to be cfg'd out when not building for the kernel target, or something: https://github.com/hawkw/mycelium/pull/59/checks?check_run_id=373309239

mystor · 2020-01-12T23:39:46Z

rebased onto master after merging #58

hawkw · 2020-01-13T20:30:58Z

the clippy CI failure looks like an issue with the CI job rather than this branch, we should probably re-run it?

hawkw

I think we need to make some changes to how we detect and report test failures before I'd feel okay about running this in CI. In particular, if a test fails by triggering a kernel oops (which currently happens on panics as well as on CPU exceptions), the test runner will hang forever instead of exiting with a test failure, which is not ideal.

I also commented on some minor nits that are not blockers, feel free to take them or leave them.

hawkw · 2020-01-13T20:32:52Z

.github/workflows/rust.yml

      uses: actions-rs/[email protected]
      with:
-        command: test
+        command: test-x64


it looks like this only runs the in-qemu kernel tests, so if we add regular Rust unit tests in any of the lib crates, they're no longer being run on CI? IMHO we should probably have separate CI jobs for the in-qemu kernel tests and for regular rust unit tests?

I don't think they were running on CI before anyway, as we were only running cargo test on the root crate. We should definitely run tests for non-root crates though.

I don't think they were running on CI before anyway, as we were only running cargo test

oh whoops, lol.

hawkw · 2020-01-13T20:35:08Z

src/arch/x86_64.rs

+pub(crate) fn qemu_exit(exit_code: QemuExitCode) {
+    let code = exit_code as u32;
+    unsafe {
+        asm!("out 0xf4, eax" :: "{eax}"(code) :: "intel","volatile");


nit, take it or leave it: i think that everywhere else, we use AT&T syntax for inline assembly. i'm fine with either, but i think we might want to agree on a project convention for consistency...since the operand ordering is reversed it can be kind of confusing IMO

(oh god does this mean we need to have a style guide)

I can use the AT&T syntax, I'm just more used to intel syntax so tend to use it by default.
I don't think we need a style guide because of inline asm, but it's good to try to be generally consistent.

i’m fine with either syntax (i actually have a slight preference for intel as well, for extremely silly reasons: [foo + bar] is far more obvious to me than...however you say that in at&t, which i actually don’t even remember).

i think existing inline asm is at&t mostly just because most of it is about one instruction that doesn’t have any memory operands and i was too lazy to type :::: “intel”. i would be absolutely fine with changing everything to intel. i’m sure @iximeow has at least one opinion about this...

oh another note is that since @iximeow is working on adding disassembly to kernel oopses using yaxpeax, and i believe yax’s formatter is intel-ish, i have a slight preference for using the same syntax in the kernel source and in oopses

hawkw · 2020-01-13T20:37:47Z

src/main.rs

+        let span = tracing::info_span!("running test", test.name);
+        let _enter = span.enter();
+        match (test.func)() {
+            Ok(_) => tracing::info!(test.name, "TEST OK"),


nit/TIOLI: since test.name is part of the parent span here, I am not convinced we need to include it in these messages as well. up to you.

hawkw · 2020-01-13T20:40:16Z

util/src/testing.rs

+        match self {
+            Ok(_) => Ok(()),
+            Err(err) => {
+                tracing::error!(?err, "test failed");
+                Err(())
+            }
+        }


I'm a little unsure why we have this rather than just having the test cases return a fmt::Debug trait object or something? but, i am not going to block this on that.

hawkw · 2020-01-13T20:41:18Z

util/src/testing.rs

+        match self {
+            Ok(_) => Ok(()),
+            Err(err) => {
+                tracing::error!(?err, "test failed");
+                Err(())
+            }
+        }


also, this is a nit, so take it or leave it, but can't this method just be

self.map_err(|err| tracing::error!(?err, "test failed"))

Yeah, I think it can

hawkw · 2020-01-13T20:45:40Z

src/main.rs

+    match wasm::run_wasm(HELLOWORLD_WASM) {
+        Ok(()) => tracing::info!("wasm test Ok!"),
+        Err(err) => tracing::error!(?err, "wasm test Err"),
+    }


nit: since we already log the output of the test case in the test harness wrapper, can't this just be

#[mycelium_util::test] fn test_wasm() { wasm::run_wasm(HELLOWORLD_WASM) }

hawkw · 2020-01-13T20:46:50Z

src/main.rs

+        let span = tracing::info_span!("running test", test.name);
+        let _enter = span.enter();


nit, TIOLI: it seems like this is something the test-case macro could generate?

hawkw · 2020-01-13T20:48:34Z

src/main.rs

+}
+
+#[cfg(test)]
+fn test_runner(tests: &[&mycelium_util::testing::TestCase]) {


take it or leave it: I feel like a large chunk of this could go in the mycelium_util::testing module and then we could just implement the qemu exit stuff here? not a blocker though.

hawkw · 2020-01-13T20:52:26Z

util-macros/src/lib.rs

+        #[test_case]
+        const #test_case_ident: ::mycelium_util::testing::TestCase = ::mycelium_util::testing::TestCase {
+            name: #test_name,
+            func: || ::mycelium_util::testing::TestResult::into_result(#ident()),


AFAICT, the test cases always return (), and fail by panicking, but we don't capture panics because we're compiling with panic="abort". this implies to me that we should either A, figure out how to implement unwinding in the kernel (sounds hard + bad), or B, change the test case impls to return Result or something and don't indicate failures by panicking, so that we can run the whole test suite and then report which tests failed, rather than panicking as soon as a single test case fails?

Or, C: we could change the way tests are run so that each individual test gets its own clean QEMU instance, and then we have a little binary on the build host that spawns those QEMUs and collates the test results. this might actually be the best choice, since it would allow tests to fail by panicking, and it would avoid the issue that the whole kernel is basically global mutable state, and would allow us to run each test in isolation without letting other tests runs (especially those that panic) to pollute the global kernel state...but, if we end up with a lot of in-qemu test cases, it might be much slower...

I tried to make it such that our tests can return results, so we can recover from failure, rather than handling panicing assertions.

It'd be nice to have each test run in it's own clean QEMU instance, but I wasn't sure what the best way to do that was, given that we don't have a good mechanism to pass info in yet. One option would be to have the default behaviour just be to print out all available tests, and have the test runner then reset & re-call the kernel with each test in order.

It's also inconvenient with how it's currently written, as tests written outside of the mycelium_kernel module don't share the same test runner, so we'd need to effectively write an entire boot routine for each integration test, etc. that we want to write.

I don't have a great answer here, unfortunately :'-(

hawkw · 2020-01-13T21:01:15Z

src/main.rs

@@ -43,6 +156,28 @@ fn panic(panic: &core::panic::PanicInfo) -> ! {
    arch::oops(&pp)


i think we should add a line to arch::oops like

#[cfg(test)] qemu_exit(QemuExitCode::Failed);

since the current oops handler will display the oops screen and then hang so the user can read the oops screen. this means that if a test fails by panicking or by triggering a CPU fault, rather than exiting and reporting that the test failed, we will hang forever (which we especially don't want in CI). if we are going to fail tests using the Rust assert macro, we definitely need to exit QEMU in the panic handler.

alternatively, we could put #[cfg(not(test))] on the call to arch::oops here, and have the panic handler exit qemu when running tests, but that means that CPU exceptions would still not result in normal test failures...

Yeah, I wanted to do this, but didn't in the first version, as your oops handler hadn't been written yet, and I didn't want to make my changes conflict :-)

@mystor

As discussed in [#59 (comment)][1], it's good to standardize on a syntax, and @mystor and I both have a minor preference for Intel over AT&T syntax. Also, when the work started in PR #61 merges, we will be showing disassembly in crash screens using Intel syntax, so we should use consistent syntax in source. This PR changes inline assembly to use Intel syntax. In cases where the inline assembly is a single instruction with no syntactic differences between AT&T and Intel syntax (e.g. "cli", "hlt", et cetera), I did not add the "intel" flag to the `asm!` macro. I can if we'd prefer this to be explicit, but the syntax is currently equivalent. [1]: #59 (comment) Signed-off-by: Eliza Weisman <[email protected]>

@mystor

* chore: change x86 inline asm from AT&T syntax to intel As discussed in [#59 (comment)][1], it's good to standardize on a syntax, and @mystor and I both have a minor preference for Intel over AT&T syntax. Also, when the work started in PR #61 merges, we will be showing disassembly in crash screens using Intel syntax, so we should use consistent syntax in source. This PR changes inline assembly to use Intel syntax. In cases where the inline assembly is a single instruction with no syntactic differences between AT&T and Intel syntax (e.g. "cli", "hlt", et cetera), I did not add the "intel" flag to the `asm!` macro. I can if we'd prefer this to be explicit, but the syntax is currently equivalent. [1]: #59 (comment) Signed-off-by: Eliza Weisman <[email protected]> * fixy fix Signed-off-by: Eliza Weisman <[email protected]>

mystor · 2020-02-08T15:23:10Z

Superceded by #65

mystor changed the title ~~in-qemu tests~~ feat(testing): in-qemu tests Jan 3, 2020

hawkw reviewed Jan 3, 2020

View reviewed changes

mystor added 2 commits January 12, 2020 18:38

initial test of testing

aebeec3

try to run in-qemu tests in gh-actions

f07c60b

mystor force-pushed the nika/testing branch from 874548c to f07c60b Compare January 12, 2020 23:39

hawkw requested changes Jan 13, 2020

View reviewed changes

hawkw mentioned this pull request Jan 20, 2020

style: change x86 inline asm from AT&T syntax to intel #63

Merged

mystor mentioned this pull request Feb 7, 2020

feat(testing): add basic section-based test declaration support #65

Merged

mystor closed this Feb 8, 2020

mystor deleted the nika/testing branch February 8, 2020 15:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(testing): in-qemu tests #59

feat(testing): in-qemu tests #59

mystor commented Jan 3, 2020

hawkw Jan 3, 2020

hawkw commented Jan 4, 2020

mystor commented Jan 4, 2020

hawkw commented Jan 4, 2020

hawkw commented Jan 4, 2020

mystor commented Jan 12, 2020

hawkw commented Jan 13, 2020

hawkw left a comment

hawkw Jan 13, 2020

mystor Jan 14, 2020

hawkw Jan 14, 2020

hawkw Jan 13, 2020

hawkw Jan 14, 2020

mystor Jan 14, 2020

hawkw Jan 14, 2020

hawkw Jan 14, 2020

hawkw Jan 13, 2020

hawkw Jan 13, 2020

hawkw Jan 13, 2020

mystor Jan 14, 2020

hawkw Jan 13, 2020

hawkw Jan 13, 2020

hawkw Jan 13, 2020

hawkw Jan 13, 2020

hawkw Jan 13, 2020

mystor Jan 14, 2020

hawkw Jan 13, 2020

mystor Jan 14, 2020

mystor commented Feb 8, 2020

	run: sudo apt-get install qemu
	run: sudo apt-get update && sudo apt-get install qemu

		let span = tracing::info_span!("running test", test.name);
		let _enter = span.enter();

		@@ -43,6 +156,28 @@ fn panic(panic: &core::panic::PanicInfo) -> ! {
		arch::oops(&pp)

feat(testing): in-qemu tests #59

feat(testing): in-qemu tests #59

Conversation

mystor commented Jan 3, 2020

Choose a reason for hiding this comment

hawkw commented Jan 4, 2020

mystor commented Jan 4, 2020

hawkw commented Jan 4, 2020

hawkw commented Jan 4, 2020

mystor commented Jan 12, 2020

hawkw commented Jan 13, 2020

hawkw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mystor commented Feb 8, 2020